Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix jetty completion handling to forward failures #6197

Merged
merged 7 commits into from
Aug 8, 2024
Merged

Fix jetty completion handling to forward failures #6197

merged 7 commits into from
Aug 8, 2024

Conversation

SamBarker
Copy link
Contributor

@SamBarker SamBarker commented Jul 25, 2024 •
edited
Loading

Description

When fixing #6143 it was observed that the Jetty client was not handling failures well. This PR fails the appropriate futures when failures are encountered.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change
  • Chore (non-breaking change which doesn't affect codebase;
    test, version modification, documentation, etc.)

Checklist

  • Code contributed by me aligns with current project license: Apache 2.0
  • I Added CHANGELOG entry regarding this change
  • I have implemented unit tests to cover my changes
  • I have added/updated the javadocs and other documentation accordingly
  • No new bugs, code smells, etc. in SonarCloud report
  • I tested my code in Kubernetes
  • I tested my code in OpenShift

manusa
manusa approved these changes Jul 25, 2024
View reviewed changes
Copy link
Member

@manusa manusa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thx!

@manusa manusa mentioned this pull request Jul 25, 2024
Closed
18 tasks
@SamBarker SamBarker force-pushed the http_continue_support branch from 7883832 to 9f94ee6 Compare July 26, 2024 01:42
@SamBarker SamBarker marked this pull request as ready for review July 26, 2024 01:42
SamBarker
SamBarker commented Jul 26, 2024
View reviewed changes
Copy link
Contributor Author

@SamBarker SamBarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to call attention to 9c33911 as it makes testing the failure call backs possible but I'm not sure if thats a direction the project wants to take the mockwebserver in. So I have kept that as isolated commit so it can be dropped without compromising the core change.

The fact that each expectFailure test takes 20s to complete gives me pause but to change that involves exposing configuration for the retry behaviour which seems like a bigger deal than a slow test.

junit/mockwebserver/src/main/java/io/fabric8/mockwebserver/dsl/ReturnOrWebsocketable.java Outdated Show resolved Hide resolved
...mockwebserver/src/main/java/io/fabric8/mockwebserver/internal/MockServerExpectationImpl.java Outdated Show resolved Hide resolved
@SamBarker SamBarker force-pushed the http_continue_support branch from 0fab3b2 to 3881386 Compare July 26, 2024 02:28
@manusa
Copy link
Member

manusa commented Jul 26, 2024

Some notes about the MockWebServer.

For version 7.0.0 we're moving it away from OkHttp.
You can check a spike/PoC of the new implementation in #5632

Regarding the failure testing (I still haven't checked exactly what you're doing), but I kind of recall writing failure expectations without the need to modify the MockWebServer.

I think these are the ones:

https://github.com/eclipse-jkube/jkube/blob/0f530e0a6340dcb3dbc3b5ea3c4af40516dd1a17/jkube-kit/helm/src/test/java/org/eclipse/jkube/kit/resource/helm/oci/OCIRegistryClientTest.java#L107-L110

https://github.com/eclipse-jkube/jkube/blob/0f530e0a6340dcb3dbc3b5ea3c4af40516dd1a17/jkube-kit/helm/src/test/java/org/eclipse/jkube/kit/resource/helm/TestMockResponseProvider.java

As I said, I haven't checked (yet) what you're trying to accomplish, but maybe this approach could be vaild for you.

@SamBarker
Copy link
Contributor Author

SamBarker commented Jul 26, 2024 •
edited by manusa
Loading

Thanks @manusa

Regarding the failure testing (I still haven't checked exactly what you're doing), but I kind of recall writing failure expectations without the need to modify the MockWebServer.

Hmm I'm maybe using a different definition of failure, I want the connection to fail rather than the request itself.

I think the current impl is broken if you specify once instead of always as it doesn't implement equals and hashcode... I've got a some changes locally but they are still failing my test so I haven't pushed them (but I've run out of time today as the kids are climbing the walls.)

@manusa
Copy link
Member

manusa commented Jul 26, 2024

AFK
I think you could still use a lower-level expectation for the MockWebServer, i.e. provide your own response, but I'd need to check.
Also, if you want the connection to fail, maybe not use the mock web server at all?
(e.g. just a plain Java HTTP server)

@SamBarker SamBarker force-pushed the http_continue_support branch 2 times, most recently from 655fdb3 to 120fbf1 Compare July 28, 2024 23:33
@rohanKanojia
Copy link
Member

@SamBarker : I might be wrong but this test failure on windows gh action job looks related to your changes.

Error:  Failures: 
Error:    OkHttpPostTest>AbstractHttpPostTest.expectFailure:179 
Expecting a throwable with cause being an instance of:
  java.net.ConnectException
but was an instance of:
  java.io.InterruptedIOException
Throwable that failed the check:

Error:    OkHttpPutTest>AbstractHttpPutTest.expectFailure:115 
Expecting a throwable with cause being an instance of:
  java.net.ConnectException
but was an instance of:
  java.io.InterruptedIOException
Throwable that failed the check:

Could you please check if this is related?

@SamBarker
Copy link
Contributor Author

@SamBarker : I might be wrong but this test failure on windows gh action job looks related to your changes.

Yeah sorry its taken a few days to organise a windows box and get it into a state to reproduce the failure. The TL;DR the exceptions propagated are completely different on windows to macos/linux. So the choices are:

  1. Disabled the tests on windows (see 2ee636f)
  2. Relax the test to just assert that the future fails.

I lean slightly to option 1 as that gives a better test for the other platforms but I'm open to better suggestions or other ways to test the behaviour.

@SamBarker SamBarker mentioned this pull request Jul 31, 2024
Merged
11 tasks
@SamBarker SamBarker force-pushed the http_continue_support branch 2 times, most recently from 0ff66e8 to ff287b7 Compare July 31, 2024 22:58
@SamBarker
Copy link
Contributor Author

@SamBarker : I might be wrong but this test failure on windows gh action job looks related to your changes.

Yeah sorry its taken a few days to organise a windows box and get it into a state to reproduce the failure. The TL;DR the exceptions propagated are completely different on windows to macos/linux. So the choices are:

  1. Disabled the tests on windows (see 2ee636f)
  2. Relax the test to just assert that the future fails.

I lean slightly to option 1 as that gives a better test for the other platforms but I'm open to better suggestions or other ways to test the behaviour.

I came up with a better option in ff287b7 realising I could use the Junit to detect the current platform and thus return the right exception type.

I've now tested the jetty client against Openshift /CRC. So I think this is good to go.

@manusa @rohanKanojia anything else you need from me before merging?

@rohanKanojia
Copy link
Member

@SamBarker : Thanks, Marc is on PTO this week. We'll try to get this merged next week.

@manusa manusa requested a review from shawkins August 5, 2024 14:34
@SamBarker SamBarker force-pushed the http_continue_support branch from ff287b7 to 2a6a7ea Compare August 5, 2024 23:26
@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Aug 5, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@SamBarker
Copy link
Contributor Author

@shawkins @manusa Anything I can do to move this forward?

shawkins
shawkins approved these changes Aug 7, 2024
View reviewed changes
Copy link
Contributor

@shawkins shawkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you @SamBarker

@manusa manusa added this to the 7.0.0 milestone Aug 8, 2024 — with automated-tasks
@manusa manusa merged commit 6074795 into fabric8io:main Aug 8, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rohanKanojia rohanKanojia rohanKanojia approved these changes

@shawkins shawkins shawkins approved these changes

@manusa manusa manusa approved these changes

@oscerd oscerd Awaiting requested review from oscerd oscerd is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@SamBarker @manusa @rohanKanojia @shawkins